Skip to content

refactor(datagrid): introduce TableRows / Row / Delta value types#930

Merged
datlechin merged 4 commits into
mainfrom
refactor/datagrid-phase4c1-table-rows
Apr 28, 2026
Merged

refactor(datagrid): introduce TableRows / Row / Delta value types#930
datlechin merged 4 commits into
mainfrom
refactor/datagrid-phase4c1-table-rows

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

Phase C.1 of the DataGrid refactor. Introduces three new value types — Row, Delta, and TableRows — alongside the existing RowBuffer / InMemoryRowProvider / RowDataStore. No callers migrated. That's C.2's job. C.1 is small and mergeable so the new shape can be reviewed without the migration churn.

Why

Today's row-data system has three tightly coupled types with inconsistent mutation paths (direct buffer writes, inout array refs, provider methods). Sort indices don't auto-invalidate. Inserted rows live in a parallel array. Provider's weak ref to RowBuffer falls through to a shared empty buffer with no error.

Phase C replaces all three with a single value type that emits explicit Delta events on mutation. The eventual controller will apply Deltas directly to NSTableView (insertRows/removeRows/reloadData(forRowIndexes:)), retiring the SwiftUI reloadVersion counter in Phase D.

What

Row + RowID

enum RowID: Hashable, Sendable {
    case existing(Int)   // ordinal in the page result
    case inserted(UUID)  // assigned at insert; survives sort, edits, undo
    var isInserted: Bool { get }
}

struct Row: Equatable, Sendable {
    var id: RowID
    var values: [String?]
    subscript(column: Int) -> String? { get set }
}

RowID.inserted replaces the parallel Set<Int> insertedRowIndices + [Int: [String?]] insertedRowData. UUID is stable across mutations.

Delta

enum Delta: Equatable {
    case cellChanged(row: Int, column: Int)        // hot path, no allocation
    case cellsChanged(Set<CellPosition>)           // multi-cell paste
    case rowsInserted(IndexSet)
    case rowsRemoved(IndexSet)
    case columnsReplaced
    case fullReplace
    static let none = Delta.cellsChanged([])
}

TableRows

Sendable struct with ContiguousArray<Row> storage. Owns rows + columns + types + metadata (columnDefaults, columnForeignKeys, columnEnumValues, columnNullable). Every mutator returns Delta:

  • edit(row:column:value:), editMany(_:)
  • appendInsertedRow(values:), appendPage(_:startingAt:)
  • remove(rowIDs:), remove(at:)
  • replace(rows:offset:)
  • updateDisplayMetadata(...)
  • static from(queryRows:columns:columnTypes:...) factory

Deliberate non-features (rationale matters for review):

  • No Equatable — full row equality on big tables is a footgun. DataGridIdentity already keys on schemaVersion.
  • No embedded PendingChanges — composition stays at the coordinator.
  • No sortIndices / display cache / isEvicted — view-state and store concerns, not row-data concerns.
  • Mutators do NOT auto-update pending — caller's job.

CellPosition: Hashable

CellPosition (defined inline at DataGridView.swift:13) was upgraded from Equatable to Hashable so it can be a Set element inside Delta.cellsChanged. Single-line change, folded into commit 1 because Delta won't compile without it.

Tests

48 cases across three new suites in TableProTests/Models/Query/:

  • RowTests (8) — RowID factories, isInserted, subscript bounds, equality.
  • DeltaTests (8) — equality across every case, none sentinel.
  • TableRows*Tests (32 across 9 suites) — construction, reads, edit, editMany, appendInsertedRow (UUID uniqueness, padding, truncation), appendPage, remove (by IDs and IndexSet), replace, metadata, and a regression that mutations preserve column metadata.

Files

File Status
TablePro/Models/Query/Row.swift NEW
TablePro/Models/Query/Delta.swift NEW
TablePro/Models/Query/TableRows.swift NEW
TablePro/Views/Results/DataGridView.swift CellPosition: EquatableHashable
TableProTests/Models/Query/RowTests.swift NEW
TableProTests/Models/Query/DeltaTests.swift NEW
TableProTests/Models/Query/TableRowsTests.swift NEW
CHANGELOG.md [Unreleased] / Changed entry

The existing types — RowBuffer, InMemoryRowProvider, RowDataStore — are not modified. They coexist with the new types in C.1 and are deleted in C.2.

Test plan

  • swiftlint lint --strict — clean
  • xcodebuild ... build — succeeds
  • xcodebuild ... test -only-testing:TableProTests/RowTests -only-testing:TableProTests/DeltaTests -only-testing:TableProTests/TableRowsTests — all green
  • Existing tests (PendingChangesTests, DataChangeManagerTests, etc.) unaffected
  • App still runs identically in dev — no callers migrated, so no behavior change should be observable

C.2 sketch (next PR, not this one)

Migration order, app green at each step:

  1. TableRowsStore + TableRowsController introduced
  2. Result delivery / load-more / ResultSet write into TableRowsStore
  3. Read paths (NSTableView delegate, JSON view, sidebar, export) switch to TableRows
  4. Cell edit goes through tableRows.edit(...) returning Delta
  5. RowOperationsManager takes inout TableRows, returns Delta
  6. Undo replay returns Delta-keyed instructions
  7. Sort moves to controller, keyed by Row.id
  8. Display cache keyed off Row.id
  9. Discard / tab switch / eviction go through TableRows lifecycle
  10. Delete RowBuffer / InMemoryRowProvider / RowDataStore + add tab-close auto-remove regression test

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8788bc5b8e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

self.columnDefaults = columnDefaults
didChange = true
}
if let columnForeignKeys, columnForeignKeys != self.columnForeignKeys {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Compare foreign keys using stable fields

updateDisplayMetadata currently decides whether foreign keys changed by comparing [String: ForeignKeyInfo] directly, but ForeignKeyInfo has a synthesized equality that includes its id UUID (QueryResult.swift), which is regenerated on each construction. In practice, reloading identical FK metadata from the backend will often look different and return .columnsReplaced, causing unnecessary full metadata invalidations/reloads even when nothing semantically changed.

Useful? React with 👍 / 👎.

@datlechin datlechin changed the title refactor(datagrid): introduce TableRows / Row / Delta value types (Phase C.1) refactor(datagrid): introduce TableRows / Row / Delta value types Apr 28, 2026
@datlechin datlechin merged commit 0129afb into main Apr 28, 2026
1 check passed
@datlechin datlechin deleted the refactor/datagrid-phase4c1-table-rows branch April 28, 2026 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant